Skip to content

fix: deduplicate learning retries across tool invocations - #250

Merged
senamakel merged 1 commit into
mainfrom
swiggy-memory-stall
Oct 10, 2026
Merged

fix: deduplicate learning retries across tool invocations#250
senamakel merged 1 commit into
mainfrom
swiggy-memory-stall

Conversation

@senamakel

@senamakel senamakel commented Oct 10, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

Repeated memory.learn calls with identical content received new item IDs because the provider-assigned invocation ID participated in StoreItem::fingerprint. In a real chat this produced duplicate learnings and a save/delete cycle instead of an answer.

Exclude only meta.tool_call.id from identity while retaining it in stored provenance. Namespace, content, confidence and tool name still distinguish items; conversation tool calls remain part of transcript identity. Existing records are preserved; this changes fingerprints for future writes carrying an invocation ID and does not migrate historical duplicates.

Validation: the regression failed against the original fingerprint and passes after the change; cargo test -p tinymemory-api --all-features passes (94 unit tests, 6 integration tests, 3 doctests); cargo clippy -p tinymemory-api --all-features --all-targets -- -D warnings passes.

Summary by CodeRabbit

  • Behavior Changes
    • Learning items now retain the same fingerprint when retried with a different tool-call invocation ID. The original ID remains stored as metadata; changes to the namespace or tool name still affect item identity.
  • Documentation
    • Clarified which metadata values are excluded from item fingerprints and how tool-call IDs relate to item identity and conversation content.

@senamakel @medullabot
fix: deduplicate learning retries across tool invocations
cefb3e9 
Co-authored-by: Medulla <medulla@tinyhumans.ai>
@tinysweeper

tinysweeper Bot commented Oct 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper reviewed this change across 6 lane(s) and found 0 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below.

State: Ready for maintainer review
Priority: none
Reviewed head: cefb3e9b2e81
Updated: 1791636241 (Unix time)

Review snapshot

Change surface Files Review signal Count
Production 1 Active findings 0
Tests 1 Noted findings 0
Documentation 1 Resolved findings 0
Configuration 0 Pending checks/questions 0

Completeness: Complete
Test assessment: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

What changed

The review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below.

Features

None identified with supported citations.

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

No active actionable findings.

Before merge

None.

Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 3 files; 0 findings. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change correctly excludes the provider-assigned metadata tool-call ID from item fingerprints while preserving the tool name and other item identity fields. The implementation and regression test look safe to merge. 1 file was not security-reviewed: docs/architecture/api-items.md (prose or tabular data). _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The fingerprint change clears `meta.tool_call.id` alongside the existing provenance exclusions, and the new test `retried_learning_ignores_the_provider_tool_call_id` pins the behaviour: differing ids hash equal, the stored id is untouched, and both the namespace and the tool name still change the fingerprint. Docs are updated in the same change. Looks sound to merge. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change matches its description: the fingerprint now excludes `meta.tool_call.id` while the stored metadata keeps it, docs and rustdoc are updated in the same commit, and the new regression test covers both the dedup and the still-distinguishing fields. No problems found. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._

e2e

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: This PR excludes meta.tool_call.id from the item fingerprint so provider retries replay instead of duplicating, with a unit test and doc updates. The unit test covers the fingerprint maths, but no end-to-end harness reaches the idempotency behaviour the change exists for — there is no e2e workflow for tinymemory-api in the tree, and the cortexdb integration files only mention the word 'memory' incidentally. _Code retrieval was unavailable (model: ladder embeddings returned 400 Bad Request: {"error":{"message":"unknown ladder vectors; known ladders are flash (also chat-v1, flash-v1), instant (also no-think, instant-v1), reasoning (also deepseek), max-reasoning (also max-reasoning-v1), deepseek-flash (also reasoning-v1, agentic-v1), deep (also luna), scribe, uncensored, vectors-oai3 (also embeddings-oai3-v1), vision (also vision-v1, multimodal-v1), image (also images-v1, image-v1), vi), so this review saw the diff alone._ _Memory was unavailable (model: cortex: v1/recall: error sending request for url (http://cortexdb:3141/v1/recall\)\), so this review ran without it._
Evidence and run details
  • Models: gpt-5.6-luna, glm-5.3-flash
  • Spend: $0.003711
  • Tokens: 64809 input · 3153 output · 4459 cached · 0 embedding
Head State Pass summary
cefb3e9b2e81 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 1791636241)

tinysweeper 0.1.0

@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c5f44d2d-3f2f-4b51-b03a-eae91f954cb4
📥 Commits

Reviewing files that changed from the base of the PR and between 11a0bb3 and cefb3e9.

📒 Files selected for processing (3)
  • crates/tinymemory-api/src/item/mod.rs
  • crates/tinymemory-api/src/item/mod_tests.rs
  • docs/architecture/api-items.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

Item fingerprints now exclude meta.tool_call.id, in addition to observed_at and observed_actor. Tests check that tool-call IDs do not affect fingerprints, while namespace and tool name changes do. Documentation describes the stored metadata and conversation content.

Changes

Item fingerprint behavior

Layer / File(s) Summary
Exclude invocation IDs from fingerprints
crates/tinymemory-api/src/item/mod.rs, crates/tinymemory-api/src/item/mod_tests.rs, docs/architecture/api-items.md		 fingerprint clears the tool-call ID from a cloned item before hashing. The test checks that changing the ID leaves the fingerprint unchanged, while changing the namespace or tool name changes it. The documentation states that stored metadata retains the original ID and conversation-turn tool calls remain part of conversation content.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: m3ga-mind

Merge Risk | ⚪ Minimal · up to cefb3

Merge Risk: ⚪ Minimal · up to cefb3

Repeated learning calls that differ only by invocation ID will now deduplicate, as intended. Historical duplicates are not migrated. No actionable merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to cefb3

The change retains namespace and content distinctions while separating invocation provenance from item identity. The principal risk is a cross-version lifecycle mismatch: retrying an existing item after upgrading can create a second persistent ID, and cleanup using only the newest receipt can leave the earlier copy. Historical IDs and scoped deletion remain available. No newly introduced authorization bypass was established.

Retained concerns

  • Low · reliability · inferred: Upgrade or rollback can split one item's retry history across persistent IDs. Cortex derives its lookup ID from the currently installed fingerprint algorithm, without reconciling historical fingerprints in the inspected path. A retry after upgrading can therefore append a new copy instead of recognizing the historical copy or completing its remaining events. Deleting only the new receipt leaves historical events untouched, creating a bounded privacy-relevant cleanup and recovery hazard. Historical-ID and matching-filter deletion remain viable recovery paths; this is not an authorization-bypass finding.
Security review details

Security Blast Radius

  • inferred — The identity transition affects document, conversation and learning records whose item-level metadata carries an invocation ID, in stores consuming this fingerprint. The supported cleanup hazard stays within the affected records and their existing storage scopes; the inspected change does not establish increased cross-tenant reach or credential authority.

Trust Boundaries and Controls

  • observed — Changing only invocation ID cannot merge records across namespaces in the tested contract. Cortex continues to construct storage paths from namespace, kind and configured root independently of that ID. These are client-side isolation facts, not verification of the external backend's tenant authorization.

Resilience and Maintainability Implications

  • observed — Historical cleanup remains possible: get and export preserve persistent IDs, ID deletion removes events carrying those IDs, and matching-filter deletion walks admitted events independently of the current fingerprint. This limits the transition concern and provides recovery mechanisms, although deleting only a newer receipt does not select an older copy.

Hardening Proposals

  • proposed — Consider an explicit upgrade and rollback procedure that preserves historical IDs for cleanup, or version-aware replay reconciliation where cross-version deduplication is required. Validate mixed-version partial writes and concurrent requests with different retained invocation IDs against the real backend before relying on fingerprint equality as an atomic uniqueness guarantee.
Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: deduplicating learning retries across different tool invocations.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit checks the call ID’s place,
Then hashes the item at its pace.
The tool name stays; the namespace too,
The stored call ID remains true.
Hops off, with fingerprints in view.

Comment @coderabbitai help to get the list of available commands.

tinysweeper[bot]
tinysweeper Bot approved these changes Oct 10, 2026
View reviewed changes

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking. Approving.

             $0.0037 · 64,809 in / 3,153 out · 4,459 cached (7%)  · gpt-5.6-luna, glm-5.3-flash
critique:    $0.0027 · 35,456 in / 1,027 out · 4,139 cached (12%) · gpt-5.6-luna
security:    $0.0009 · 10,007 in / 344 out   · 0 cached (0%)      · gpt-5.6-luna
tests:       $0.0000 · 4,978 in  / 145 out   · 64 cached (1%)     · glm-5.3-flash
description: $0.0000 · 4,517 in  / 65 out    · 64 cached (1%)     · glm-5.3-flash
e2e:         $0.0001 · 6,339 in  / 716 out   · 64 cached (1%)     · glm-5.3-flash

@tinysweeper tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Oct 10, 2026
chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Oct 10, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cefb3e9b2e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/tinymemory-api/src/item/mod.rs
Comment on lines +283 to +284
if let Some(call) = identity.meta_mut().tool_call.as_mut() {
call.id = None;

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve IDs for existing tool-call items

When upgrading a store containing an item with meta.tool_call.id, that item remains labeled with the previous fingerprint, which included the ID, while this code now computes a different fingerprint after clearing it. CortexEngine::store_items looks up only the newly computed ID, so re-storing the same learning after an upgrade writes a duplicate instead of returning the existing record as a replay; reconstructed items also no longer satisfy the documented id == item.fingerprint() invariant. Add a compatibility lookup or migration for the legacy fingerprint before changing this public identity behavior.

AGENTS.md reference: AGENTS.md:L248-L249

Useful? React with 👍 / 👎.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 10, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-10T12:45:43.884241Z cefb3e9 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@senamakel senamakel mentioned this pull request Oct 10, 2026
Merged
6 of 7 tasks
@senamakel
senamakel merged commit 15a6114 into main Oct 10, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments
@coderabbitai coderabbitai[bot] coderabbitai[bot] approved these changes
+1 more reviewer
@tinysweeper tinysweeper[bot] tinysweeper[bot] approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@senamakel